Skip to content

Conversation

@cjyetman
Copy link
Member

@cjyetman cjyetman commented Mar 6, 2025

@cjyetman cjyetman marked this pull request as ready for review March 9, 2025 08:57
@cjyetman cjyetman requested a review from jacobvjk as a code owner March 9, 2025 08:57
@cjyetman cjyetman requested a review from jdhoffa March 9, 2025 08:57
Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly seems reasonable, but I do want to highlight that adding this function here should probably lead to an update downstream here: https://github.com/RMI-PACTA/pacta.multi.loanbook/blob/main/R/run_calculate_match_success_rate.R
At the same time, we should probably validate that the features relied upon in the pacta.multi.loanbook version are still available or that they can be introduced easily in p.m.l. when switching to this implementation

@jdhoffa
Copy link
Member

jdhoffa commented Mar 10, 2025

mostly seems reasonable, but I do want to highlight that adding this function here should probably lead to an update downstream here: https://github.com/RMI-PACTA/pacta.multi.loanbook/blob/main/R/run_calculate_match_success_rate.R At the same time, we should probably validate that the features relied upon in the pacta.multi.loanbook version are still available or that they can be introduced easily in p.m.l. when switching to this implementation

To that end, I would suggest opening a similar PR in pacta.multi.loanbook that depends on this PR, to ensure that the required functionality is reproduced, and we can deprecate the functionality from PML in favour of this.

@cjyetman
Copy link
Member Author

PML substantially complicates this by allowing for calculating the match success rates for multiple loanbooks/groups at a time, a concept which is mostly irrelevant for r2dii.match. That could be addressed in 2 ways...

  1. introduce the grouping concept here and deal with the consequence of documenting/explaining what a group ID is for even though it's conceptually irrelevant
  2. create some convoluted process in PML that splits the groups out separately and runs them through r2dii.match::calculate_match_success_rate() and then recombines them

any preference?

@jacobvjk
Copy link
Member

jacobvjk commented Mar 10, 2025

PML substantially complicates this by allowing for calculating the match success rates for multiple loanbooks/groups at a time, a concept which is mostly irrelevant for r2dii.match. That could be addressed in 2 ways...

  1. introduce the grouping concept here and deal with the consequence of documenting/explaining what a group ID is for even though it's conceptually irrelevant
  2. create some convoluted process in PML that splits the groups out separately and runs them through r2dii.match::calculate_match_success_rate() and then recombines them

any preference?

as much as I don't want to convolute the pml processes, I think it makes more sense to keep this one here as simple as possible and not introduce the "group_id", which would be completely leftfield in the r2dii.*/pacta.loanbook context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: gains function calculate_match_success_rate()

4 participants